[Suggestion] runDeferredTask interpolates err.message into a string before passing it to debugLogger.warn, which drops the stack trace. The debugLogger's formatArgs already handles Error objects specially (extracting arg.stack), but it never sees the original error here.

— qwen3.7-max via Qwen Code /review

Addressed. runDeferredTask now passes the original error object to the logger:

debugLogger.warn(`${name} failed:`, err);

This preserves stack traces through formatArgs, and the startup prefetch tests assert the logger receives the original Error object.

[Suggestion] api_preconnect records startup_prefetch_started but never emits the corresponding startup_prefetch_completed or startup_prefetch_failed event. All post-render tasks go through runDeferredTask which records all three lifecycle events. This asymmetry means profiling consumers see api_preconnect as perpetually in-flight.

Consider either wrapping the call in runDeferredTask for consistency, or removing the manual recordStartupEvent here so only runDeferredTask-managed tasks appear in the startup_prefetch_* event stream.

— qwen3.7-max via Qwen Code /review

Addressed. The manual startup_prefetch_started event for api_preconnect has been removed, so only tasks managed by runDeferredTask enter the balanced startup_prefetch_* lifecycle stream.

There is also a regression test asserting that api_preconnect does not emit an unbalanced lifecycle event.

[Suggestion] The original code in gemini.tsx guarded housekeeping with if (config.isInteractive()) and a comment explaining that non-interactive modes don't create the file-history dirs this cleans. That guard was dropped in the migration.

This is safe today since startPostRenderPrefetches is only called from startInteractiveUI, but the function's signature accepts any Config — a future non-interactive caller would silently start housekeeping. Consider restoring the guard for defense-in-depth, or adding a comment noting the implicit invariant.

— qwen3.7-max via Qwen Code /review

Addressed. background_housekeeping is now guarded by config.isInteractive() inside startPostRenderPrefetches(), preserving the previous invariant even if a future non-interactive caller invokes this helper directly.

The startup prefetch tests cover the non-interactive case and assert housekeeping is not scheduled.

[Suggestion] telemetry_init is the only synchronous task among the deferred tasks. initializeTelemetry(config) runs as a single microtask inside Promise.resolve().then(task), blocking the event loop until OTel SDK setup completes (resource attributes, exporters, BatchSpanProcessor, PeriodicExportingMetricReader). Every other deferred task starts with await import(...) which yields to the event loop before doing real work.

This has two consequences:

1. Performance: The sync microtask can delay React's post-mount useEffect state updates and pending I/O, partially undermining the "fast time-to-interactive" goal.
2. Ordering dependency: Because telemetry_init is declared before ide_connect and runs synchronously, it completes before connectIdeForStartup → logIdeConnection executes. logIdeConnection guards on isTelemetrySdkInitialized() and silently drops the event if false. This ordering is accidental — if initializeTelemetry is ever made async or the task declarations are reordered, IDE connection telemetry events will be silently dropped with no test catching it.

Making this async with a dynamic import (consistent with the other tasks) yields to the event loop before running, and also removes the static initializeTelemetry import at the top of the file. To preserve the ordering guarantee with ide_connect, either sequence them explicitly or add a comment documenting the dependency.

— qwen3.7-max via Qwen Code /review

Not addressed in this commit.

I kept telemetry_init synchronous for now because changing it to an async dynamic import also changes the implicit ordering with deferred ide_connect; logIdeConnection currently depends on telemetry already being initialized. Fixing this properly should sequence telemetry and IDE connection explicitly, or introduce a shared readiness contract, instead of only adding an async import and risking dropped IDE telemetry events.

This should be handled as a separate follow-up so the scheduling and telemetry-ordering behavior can be reviewed together.

[Suggestion] The deferred IDE connection runs fire-and-forget via runDeferredTask with no timeout. If the IDE IPC endpoint is unresponsive (VS Code extension host restarted, stale socket file, JetBrains backend overloaded), the connect() promise hangs indefinitely inside runDeferredTask. The catch handler only fires on rejection, not on hang. The result: startup_prefetch_started with name: ide_connect is recorded but neither completed nor failed ever fires, and there is no diagnostic trail explaining why IDE context is missing.

This is a new failure mode introduced by the PR. The old code awaited connect() inside initializeApp() before first paint — a hang was immediately visible as "the TUI never appears." The new code makes the same hang invisible.

Consider wrapping the connection in a timeout (e.g., Promise.race with a 10s setTimeout that rejects), so the profiler captures the outcome and debugLogger.warn fires on timeout.

— qwen3.7-max via Qwen Code /review

Addressed in 6cba2c578.

Deferred ide_connect is now wrapped in a 10s timeout. If the IDE IPC promise hangs, the timeout rejects through runDeferredTask, so the profiler records startup_prefetch_failed and the debug logger emits a clear timeout diagnostic instead of leaving the task permanently in-flight.

A unit test covers the hanging connection case.

[Suggestion] The telemetry deferral decision is split across two files with no type-level or comment-level link:

• cli/config/config.ts decides whether to skip eager init: deferTelemetryInitialization: interactive && !isAcpMode && !question
• This line decides whether to run post-render init: postRenderInitializeTelemetry: !input

The invariant deferTelemetryInitialization === postRenderInitializeTelemetry must hold for the interactive TUI path, or telemetry either never initializes (silent data loss) or initializes twice (masked today by the telemetryInitialized guard in sdk.ts, but that guard is an implementation detail that could change).

Today the conditions are equivalent (interactive && !isAcpMode && !question simplifies to !question on the interactive path, matching !input). But this relies on three unstated premises: ACP mode never reaches this branch, question and input are the same value, and no future condition is added to either side independently.

Consider adding a linking comment at both sites, or computing the deferral flag once and passing it through to both loadCliConfig and startInteractiveUI.

— qwen3.7-max via Qwen Code /review

Addressed in 6cba2c578 with the Config-backed option.

Config now stores the telemetry deferral decision as telemetryInitializationDeferred, and startInteractiveUI() defaults post-render telemetry startup from config.isTelemetryInitializationDeferred() instead of recomputing a separate !input condition in gemini.tsx.

This keeps the eager-skip and post-render-init decisions tied to the same source of truth.

[Suggestion] This test verifies deferTelemetryInitialization: false for prompt-interactive, but there is no corresponding positive test verifying deferTelemetryInitialization: true for the ordinary interactive case (no -p, no -i "prompt", not ACP). The positive path is the primary motivation for the feature — deferring telemetry for ordinary TUI startup — yet it has no test. A regression that accidentally always sets false would go undetected.

Consider adding a test with process.argv = ['node', 'script.js', '-i'] (interactive, no question) that asserts deferTelemetryInitialization: true.

— qwen3.7-max via Qwen Code /review

Addressed in 6cba2c578.

Added a positive loadCliConfig test for ordinary interactive startup that sets process.stdin.isTTY = true, uses no initial prompt, and asserts deferTelemetryInitialization: true is passed to Config.

[Critical] deferIdeConnection only treats a CLI question as an immediate first request, but interactive sessions with --input-file can submit as soon as RemoteInputWatcher.setSubmitFn() is installed. Since IDE connection is now fire-and-forget post-render, an IDE-enabled driver can send that first request before IDE context is connected, regressing the same first-request guarantee this PR preserves for prompt-interactive startup.

— gpt-5 via Qwen Code /review

Addressed in 6cba2c578.

deferIdeConnection now treats config.getInputFile?.() as an immediate interactive-submit source, so interactive sessions using --input-file keep the IDE connection awaited before the first remote prompt can run. The post-render prefetch receives connectIde: false in that case to avoid a duplicate connection attempt.

Added a gemini.test.tsx regression test for interactive + input file.

[Critical] [build] fallbackModel is missing from this mock CliArgs object. The same omission exists at line 1511. CliArgs.fallbackModel is required (no ?), so tsc --build reports TS2345 on both lines. Line 1267 already includes fallbackModel: undefined — these two were missed.

— qwen3.7-max via Qwen Code /review

Addressed in 6cba2c578.

Both affected mock CliArgs objects now include fallbackModel: undefined, and packages/cli typecheck passes with tsc --noEmit.

[Suggestion] handleAutoUpdate is mocked (line 20) and wired into the handleAutoUpdate module mock (line 46), but no test asserts it was actually called. This test verifies checkForUpdates was called but skips the second half of the update pipeline — a regression that drops handleAutoUpdate from the .then() chain would pass all tests.

— qwen3.7-max via Qwen Code /review

Not addressed in this commit.

I did not add this assertion because the current test intentionally uses a never-resolving checkForUpdates() promise to verify that startPostRenderPrefetches() starts work without awaiting completion. In that specific test, handleAutoUpdate should not run.

A separate test that resolves checkForUpdates() and asserts the handleAutoUpdate(info, settings, projectRoot) handoff would be useful, but it is a test-strengthening follow-up rather than part of the first-request correctness fixes in 6cba2c578.

[Suggestion] The default options.postRenderConnectIde ?? true makes post-render IDE connection opt-out. Combined with initializeApp()'s default deferIdeConnection: false (eager), any future caller of startInteractiveUI that omits options will get a double IDE connection: once awaited in initializeApp, once fire-and-forget in startPostRenderPrefetches. IdeClient.connect() has no re-entry guard.

The sole production caller in gemini.tsx passes the option correctly, masking this footgun.

Changing the default to false makes the safe/already-connected state the default, matching initializeApp's eager-connect behavior. The gemini.tsx caller already passes postRenderConnectIde: deferIdeConnection explicitly, so no caller change is needed.

— qwen3.7-max via Qwen Code /review

Addressed in d05359056.

startInteractiveUI() now defaults postRenderConnectIde to false, so the already-connected/eager IDE path is the safe default and future callers that omit options will not accidentally connect twice. The production caller in gemini.tsx still passes postRenderConnectIde: deferIdeConnection explicitly, so ordinary TUI startup keeps the deferred IDE behavior when intended.

Updated startInteractiveUI tests to assert the safer default. Verified with:

cd packages/cli && npx vitest run src/gemini.test.tsx
cd packages/cli && npm run typecheck
git diff --check

[Suggestion] withTimeout masks the real error when the timeout fires first. After Promise.race resolves with the timeout rejection, the original promise is still running — and if it eventually rejects, that rejection is silently consumed (no .catch() attached to the losing branch). The debug log will only show the timeout error, losing the actual root cause.

Consider attaching a .catch() to the losing promise to surface its error:

return Promise.race([promise, timeout]).finally(() => {
  clearTimeout(timeoutId);
});

Alternatively, log the underlying error in a separate .catch() on promise itself (e.g., promise.catch(err => debugLogger.debug(\${name} underlying error:`, err))`) so it doesn't mask the timeout but still surfaces the real cause.

Addressed in 95c5f6c03.

I added a losing-promise catch in the deferred timeout helper so if ide_connect times out first and the underlying IDE connection later rejects, the original error is logged at debug level without changing the externally observed timeout failure.

[Suggestion] The deferred IDE connection creates a race window for IDE extension users: the TUI renders with an interactive input area before the IDE IPC handshake completes. If a user types and submits a message during that window, the model request goes out without IDE context (workspace info, editor state).

For IDE extension sessions specifically, consider showing a brief "Connecting to IDE..." status indicator until ide_connect completes, or deferring input availability until the deferred IDE connection resolves. Ordinary CLI sessions are unaffected since connectIde is opt-in.

Addressed in 95c5f6c03 with the status-only approach.

Ordinary interactive IDE TUI startup now emits deferred IDE connection lifecycle status and surfaces it in the footer: connecting shows IDE connecting... context may be unavailable, success clears the indicator, and failure/timeout leaves a warning. This keeps input available and preserves the post-render startup performance tradeoff while making the race window visible to users.

[Suggestion] Two accepted tradeoffs converge at this line, and it would help future readers to document both:

1. Auth telemetry gap: auth-related events emitted between performInitialAuth() (which runs eagerly in initializeApp) and the deferred initializeTelemetry() call are no-ops. This is acknowledged in the PR risk section.
2. Asymmetric deferral conditions: deferIdeConnection (in gemini.tsx) considers question (prompt-interactive doesn't defer IDE), but deferTelemetryInitialization here does not check question. The design rationale is sound — prompt-interactive needs IDE context for the auto-submitted prompt but doesn't need telemetry before the first request — but a cross-reference comment would prevent future maintainers from "fixing" the asymmetry.

Addressed in 95c5f6c03.

I documented the accepted telemetry deferral tradeoffs at the deferTelemetryInitialization decision point: auth events before deferred telemetry init may be no-ops, and the telemetry condition is intentionally not identical to IDE deferral because prompt-interactive needs IDE context before auto-submit while telemetry can remain post-render unless an initial prompt is present.

[Suggestion] handleAutoUpdate(info, settings, config.getProjectRoot()) performs synchronous file-system I/O (writing update-notification files). Inside a fire-and-forget deferred task, this blocks the event loop briefly. For small files this is acceptable, but it's worth documenting the assumption that handleAutoUpdate is expected to be fast — or wrapping it in a Promise.resolve() boundary so it doesn't block other microtasks queued by the prefetch batch.

Not changed in this pass.

handleAutoUpdate still runs inside the post-render fire-and-forget task, and the synchronous work here is limited to small update-notification file I/O after first paint. Wrapping it in another microtask would not remove the synchronous I/O cost, and there is no current evidence that it causes visible startup contention. I would keep this as a follow-up only if profiling shows the update file write is material.

[Suggestion] No test in this suite asserts that recordStartupEvent('startup_prefetch_completed', { name }) is actually called by runDeferredTask. The production code does this correctly (line 56), but a profiler lifecycle test would catch regressions if someone restructures the .then() chain. Consider adding an assertion like:

expect(recordStartupEvent).toHaveBeenCalledWith(
  'startup_prefetch_completed',
  expect.objectContaining({ name: 'update_check' }),
);

Addressed in 95c5f6c03.

I added a successful deferred update_check lifecycle test that asserts both startup_prefetch_started and startup_prefetch_completed are recorded, so the profiler completion contract is now covered alongside the failure path.

[Suggestion] connectIdeForStartup is tested at line 189 with getIdeMode()=true, but the early-return path (getIdeMode()=false → skip connection) is only tested indirectly through initializeApp at line 200. A dedicated unit test calling connectIdeForStartup(mockConfig) with getIdeMode()=false and asserting no IDE client calls would better document the contract and catch regressions if the guard condition changes.

Addressed in 95c5f6c03.

I added a direct connectIdeForStartup() early-return test for getIdeMode() === false, asserting that IdeClient.getInstance(), connect(), and logIdeConnection() are not called. This locks the shared helper contract directly instead of only covering it through initializeApp().

[Suggestion] When withTimeout fires at 10s, the underlying connectIdeForStartup promise is not cancelled — it continues running. If the IDE connection succeeds after the timeout (e.g., at second 11-12), IdeClient becomes connected and workspace data flows, but the UI permanently shows "IDE connection unavailable" because no connected status event is emitted after the timeout path. The failed state in StartupIdeConnectionStatus is terminal — there is no late-success recovery or disconnect on timeout.

This creates a state inconsistency where the footer claims the IDE is unavailable while it is actually connected and functional. Users may disable IDE integration or file bugs based on the misleading status.

Consider either:

1. Disconnecting the IDE client on timeout so the internal state stays consistent with the UI, or
2. Keeping a reference to the underlying promise and emitting { state: 'connected' } if it eventually succeeds after the timeout.

— qwen3.7-max via Qwen Code /review

[Suggestion] The test suite covers the timeout scenario (hanging promise + 10s advance) and the success path, but there is no test for a quick IDE connection failure before the timeout fires. This is a distinct code branch: the catch block at the end of the ide_connect task extracts the error message via err instanceof Error ? err.message : String(err) and emits { state: 'failed', message }. A quick-rejection test (e.g., mockConnectIdeForStartup.mockRejectedValue(new Error('connection refused'))) would verify this path and the error message extraction logic.

it('fails deferred IDE connection when the startup connect rejects quickly', async () => {
  const config = makeConfig();
  mockConnectIdeForStartup.mockRejectedValue(new Error('connection refused'));
  const { statuses, stop } = captureIdeConnectionStatuses();

  try {
    startPostRenderPrefetches(config, makeSettings(), { connectIde: true });
    await vi.dynamicImportSettled();

    expect(statuses).toEqual([
      { state: 'connecting' },
      { state: 'failed', message: 'connection refused' },
    ]);
    expect(mockRecordStartupEvent).toHaveBeenCalledWith(
      'startup_prefetch_failed',
      { name: 'ide_connect' },
    );
  } finally {
    stop();
  }
});

— qwen3.7-max via Qwen Code /review